/cc @sanposhiho

/cc @alculquicondor
Do we need /lead-opt-in at #3243?

@wojtek-t (or @alculquicondor) do we need PRR review in this case? (I suppose Yes?)

/retitle KEP-3243: Update the design to mutate the label selector based on matchLabelKeys at api-server instead of the scheduler handling it

cc @dom4ha

I appreciate your comment.
I've addressed.

/assign @wojtek-t

for a PRR part reviewing. Please assign another person if needed.

for a PRR part reviewing. Please assign another person if needed.

Queued - although I will wait for SIG approval to happen first.

@sanposhiho
Thank you for your comments.

I have a question.
I understand that kube-scheduler has two roles:

1. Read matchLabelKeys of cluster-level default constraints from scheduler configuration, get key-value labels corresponding to matchLabelKeys from pod labels, and logically combine them with labelSelector internally to schedule pod.
2. Check if the key-value labels corresponding to matchLabelKeys are set in labelSelector by kube-apiserver or users, and if not, logically combine them with labelSelector internally to schedule pod.

Are my understandings correct?
I'm concerned about 2.

For example, if the following Pod manifest is applied

apiVersion: v1
kind: Pod
metadata:
  name: sample
  labels:
    app: sample
...
  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector: {}
    matchLabelKeys:
    - app

kube-apiserver will usually merge key-value labels corresponding to matchLabelKeys into labelSelector, so kube-scheduler will not handle matchLabelKeys.

  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector:
      matchExpressions:
+      - key: app
+        operator: In
+        values:
+        - sample
    matchLabelKeys:
    - app

It is the same if user sets key-value labels corresponding to matchLabelKeys in labelSelector

apiVersion: v1
kind: Pod
metadata:
  name: sample
  labels:
    app: sample
...
  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector:
      matchExpressions:
      - key: app
        operator: In
        values:
        - sample
    matchLabelKeys:
    - app

However, if key-value labels corresponding to matchLabelKeys are not merged into labelSelector by kube-apiserver for some reason, kube-scheduler will get key-value labels from pod labels and logically combine them with labelSelector internally to schedule pod.(I'm wondering if this case should be considered.)

  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector: 
      matchExpressions:
      - key: foo
        operator: In
        values:
        - bar
    matchLabelKeys:
    - app

As my suggested changes above imply, the scheduler only has one role, which is to handle matchLabelKeys within the cluster wide default constraints. In other cases (like a user created a Pod with matchLabelKeys), we assume kube-apiserver handles it and kube-scheduler doesn't have to worry about matchLabelKeys, right?

... and you know what, I found I forgot about the update path issue that I raised myself while I'm writing this comment. 😅
I'll revisit my suggestions made above.

So, answering your questions,

However, if key-value labels corresponding to matchLabelKeys are not merged into labelSelector by kube-apiserver for some reason, kube-scheduler will get key-value labels from pod labels and logically combine them with labelSelector internally to schedule pod.(I'm wondering if this case should be considered.)

Usually, no. Like my above strikethrough-ed comment argued, we can just assume kube-apiserver handles all matchLabelKeys that users added. And, the scheduler has to only worry about the cluster wide default constraints.
HOWEVER, there's an exception which is during the cluster upgrade. We need to consider scenarios kube-apiserver not handling matchLabelKeys because, at the time running the cluster upgrade, there could be unscheduled pods that are made through the old kube-apiserver, and have to be handled by the new version kube-scheduler. In this case, kube-scheduler needs to handle matchLabelKeys even though those matchLabelKeys came from users, not from the cluster wide default constraints.
That being said, after one release with this new design, so basically at the next release cycle, we can change kube-scheduler to only handle matchLabelKeys from the cluster wide default constraints.

@sanposhiho
I've addressed them and add description related to kubernetes/kubernetes#129480 (comment)

@sanposhiho
Thank you, I've incorporated your suggestion.

@dom4ha @macsko
Thank you for your comments.
I've addressed and please check them.

lgtm from me. Let's wait for @dom4ha

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, mochizuki875, sanposhiho

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/lgtm

@wojtek-t We haven't modified PRR file and it's merged without your approval.
Please take a look and we can follow up accordingly if you have any suggestion.

No further concerns from my side. I have limited availability this week, so thanks @macsko and @alculquicondor for approvals